@W-19444022@ E2E tests base level #121

kylewalke · 2025-09-02T19:33:01Z

What does this PR do?

Adding E2E tests with Playwright to the repo

What issues does this PR fix or reference?

@W-19444022@

mshanemc

meta: please run npx knip to cleanup unused exports. I don't see a lot of refactoring cleanups or "clean up things while doing another fix/feature", so if we let in a bunch of slop and dead code, it'll be there for a long time.

mshanemc · 2025-09-03T19:12:58Z

.github/workflows/e2e-tests.yml

+
+on:
+  push:
+    branches: [main, kyledev/e2eTesting]


you want to clean this up, right?

Yes, I need it to be there for the duration of the branches existence (this PR) but it would be removed after the fact since this branch will get deleted.

mshanemc · 2025-09-03T19:19:40Z

e2e-tests/README.md

+│   ├── outline-helpers.ts     # Outline view specific helpers
+│   └── global.ts              # Global setup and teardown functions
+├── test-server.js             # VS Code Web test server
+├── playwright.config.ts       # Playwright configuration


I don't think I need this AI slop. Maybe only point out the interesting/unexpected parts?

file/folder names should be doing most of the work.

And trying to maintain it for every file is gonna suck.

Yeah, I can reduce it down to a more general overview and less nitty gritty that will change a lot over time. It's nice to see while working on it but I'm totally ok not immortalizing it.

mshanemc · 2025-09-03T19:29:10Z

packages/apex-ls/src/utils/EnvironmentUtils.ts

-  return typeof globalThis !== 'undefined' && 
-         'indexedDB' in globalThis && 
-         (globalThis as any).indexedDB !== null;
+  return (


is that a real scenario (browser without indexDB?)

Browser no but desktop wouldn't have it, right?

for context, these were all linting auto-fixes I noticed needed auto-fixing while workin on the e2e tests so no functional changes in the apex-ls package files.

my point is that IndexedDBStorage is only called within BrowserStorageFactory so why have the check?

To prevent us from accidentally making the mistake of trying to use indexDB outside of the browser?

That seems extra (writing code that wraps a runtime failure around another runtime failure). Like, there's not a scenario where that would be shippable, right?

my point is that IndexedDBStorage is only called within BrowserStorageFactory so why have the check?

For the same reason we null check variables we know we've initialized that get passed into methods I guess. Is it overkill? Is it just safety? 🤷 I'm ok to remove the check if you think it's unnecessary.

To prevent us from accidentally making the mistake of trying to use indexDB outside of the browser?

yes

That seems extra (writing code that wraps a runtime failure around another runtime failure). Like, there's not a scenario where that would be shippable, right?

Also yes.

For the same reason we null check variables we know we've initialized that get passed into methods I guess
no, that's the reason we use TS in strict mode. If it can't take a null, the compiler should catch it...shifting left (from runtime or test-time errors to compiler errors).

I don't think there's any value in moving from runtime error to runtime error but I can approve the PR with this if you have string feelings about it.

e2e-tests/utils/test-helpers.ts

mshanemc · 2025-09-03T19:39:45Z

e2e-tests/utils/test-helpers.ts

+    // Hover to show file selection in debug mode
+    if (process.env.DEBUG_MODE) {
+      await clsFile.hover();
+      await page.waitForTimeout(500);


I'd like to avoid waitForTimeout. It's hard to keep Cursor from doing it, but the tests should be waiting for some screen element, etc.

I can abide by that. It's really easy/convenient but introduces inaccurate cues for when to move on to the next step of a test. I can remove them in place of more precise triggers.

mostly it makes the tests waste time for no good reason, and the faster these are the less of our time it wastes

mshanemc · 2025-09-03T19:47:28Z

e2e-tests/utils/global.ts

+  }
+
+  // Create test workspace using fixtures
+  const workspacePath = path.resolve(__dirname, '../test-workspace');


are all the tests going to try to use the same workspace? would it make more sense to have a helper so that a test can set up its workspace (preferably in /tmp) with the files it needs?

Yes. The setup I have right now uses the same directory that is created as the test workspace in the e2e-tests directory so that it's also easy to find. Is there additional benefit to the test workspace and files living in tmp? I think for the base e2e's this is fine but I'd be okay expanding the setup to move the workspace later.

e2e-tests is fine. I think the main thing is getting them into different directories for each test to avoid collisions. give them a guid or timestamp or test name or something to distinguish.

more of a "do I have to clean all these up if I'm running a lot of them locally" ?

mshanemc · 2025-09-03T19:48:03Z

e2e-tests/utils/global.ts

+
+  // Create test workspace using fixtures
+  const workspacePath = path.resolve(__dirname, '../test-workspace');
+  if (!fs.existsSync(workspacePath)) {


you can just mkdir without checking for existence. That's one of the nice things about recursive

I'm not generally a fan of globalSetup stuff, hopefully it can be removed

I didn't know that about mkdir! nice, okay I can clean that up. Do you mean you don't like setup steps at all or just like the globalSetup naming scheme?

I think tests should do their setup. If there's common things, those become shared fns.

globalSetup for all tests is weird because now they have to all agree on what's needed. And if you want to run one small test, (ex: click if from the vscode ui) it has to run the global setup whether it needed it or not.

mshanemc · 2025-09-03T19:49:47Z

e2e-tests/utils/global.ts

+ *
+ * @param _config - Playwright configuration
+ */
+export async function globalSetup(_config: FullConfig): Promise<void> {


unused? why have it?

mshanemc · 2025-09-03T19:56:12Z

e2e-tests/fixtures/apex-samples.ts

+/**
+ * Sample Apex class with basic methods for testing language features.
+ */
+export const HELLO_WORLD_CLASS: SampleFile = {


probably 1 file per sample class? other than apex-samples what else would there be in fixtures since this is an Apex LS?

And it's weird to have these separated from the outline-helpers stuff. Like, what if some of these had the EXPECTED_APEX_SYMBOLS so that anything using them could access both, and when you change something here you don't have to change the other file?

Come to think of it, this is a good point. There's not gonna be much else we need to generate so I think this could get merged into the other helpers.

mshanemc · 2025-09-03T19:58:19Z

e2e-tests/fixtures/apex-samples.ts

+/**
+ * Sample Apex trigger for testing trigger-specific functionality.
+ */
+export const ACCOUNT_TRIGGER: SampleFile = {


are you actually using these for anything? I see helloWorld and complex used but not the soql/trigger.

I was thinking I might add some tests to make sure that the different file ending types worked. like .trigger, .cls, and .apex but I didn't flesh out those test cases. This is an artifact from that. I want to get one of those in eventually to make sure the outline and extension activation works for all 3 filetypes still but doesn't need to be part of this initial work.

- Fix Playwright webServer configuration to use correct working directory - Add dynamic cwd detection to run npm scripts from root when executed from e2e-tests directory - Update GitHub Actions workflow to properly handle directory context - Ensure test:e2e:server script can be found and executed correctly This resolves the remaining e2e test failures by ensuring the webServer can properly locate and execute the test server startup script.

- Set headless: true in CI environments to avoid X11 display issues - Add CI-specific Chrome launch arguments for better stability - Keep headed mode for local development debugging - Resolves browser launch failures in GitHub Actions runners This fixes the 'Missing X server or $DISPLAY' error that was preventing the test server from starting in CI environments.

mshanemc · 2025-09-08T14:07:19Z

.github/workflows/e2e-tests.yml

+          npm run compile
+          npm run bundle
+          echo "🔧 Building VS Code extension..."
+          cd packages/apex-lsp-vscode-extension


I'm not a cd fan because it becomes state you hae to keep track of. -w on npm commands to run certain commands in a certain workspace.

If you really need the ls output (?) then you can also give it a directory

I've not noticed/used that before so I can definitely add it to the commands.

mshanemc · 2025-09-08T14:11:27Z

.github/workflows/e2e-tests.yml

+          retention-days: 30
+          if-no-files-found: warn
+
+      - name: Debug artifact directories


is this permanently needed? do you want all the extra ai-slop emoji debugs?

Definitely not. I should remove all that junk so it's not muddying up logs.

mshanemc · 2025-09-08T14:13:25Z

e2e-tests/tests/apex-extension-core.spec.ts

+
+    // Assert success criteria using constants
+    expect(criticalErrors.length).toBeLessThan(
+      ASSERTION_THRESHOLDS.MAX_CRITICAL_ERRORS,


I think it would be better with an allowList of certain exceptions that we know we don't care about

mshanemc · 2025-09-08T14:18:21Z

e2e-tests/utils/constants.ts

+  VS_CODE_STARTUP: 12_000,
+  LSP_INITIALIZATION: 8_000,
+  SELECTOR_WAIT: 30_000,
+  ACTION_TIMEOUT: 15_000,


Crufty the clown strikes again. Will remove.

mshanemc · 2025-09-08T14:19:17Z

e2e-tests/utils/constants.ts

+/**
+ * Test environment configuration.
+ */
+export interface TestEnvironment {


mshanemc · 2025-09-08T14:20:13Z

e2e-tests/utils/constants.ts

+  'Long running operations during shutdown',
+  'lifecycle',
+  'hostname could not be found',
+  'textDocument/diagnostic failed', // Known VS Code Web LSP issue


can you add a comment to provide the link for the known issue?

mshanemc · 2025-09-08T14:34:51Z

e2e-tests/utils/test-helpers.ts

+  // Import the content from constants to avoid duplication
+  const { APEX_CLASS_EXAMPLE_CONTENT } = require('./constants');
+
+  return createSampleFile(


I think it's just an object, the fn/type aren't doing much (default description? why not make desc required?), unless you've got a lot of plans for it.

I think it's ok to remove.

mshanemc · 2025-09-08T14:37:13Z

e2e-tests/tsconfig.json

@@ -0,0 +1,25 @@
+{
+  "compilerOptions": {
+    "target": "ES2020",


if you're node20, https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping#node-20

mshanemc · 2025-09-08T14:40:23Z

package.json

    "test:coverage:report": "node scripts/merge-coverage.js",
    "test:integration": "turbo run test:integration",
    "test:web": "node scripts/test-web-ext.js web",
+    "test:e2e": "npm run rebuild && cd e2e-tests && npx playwright test",


what's the reason for always doing a rebuild?

same comment about cd vs. -w

My imagining of the workflow would be.

Dev is doing dev on one of the packages

Dev locally verifies changes with vscode test web or in the IDE

Dev makes more changes until they reach the point at which they are ready to submit for PR

Dev runs e2e test locally which makes sure that the most updated build version is tested.

In CI, step 4 isn't really an issue that needs addressing but locally it's possible to make changes and then run the e2e test which would have it screw up. I can remove the rebuild if you think the CI job would be better off without it.

mshanemc · 2025-09-08T14:46:56Z

package.json

+    "test:e2e:debug": "npm run rebuild && cd e2e-tests && npx playwright test --debug --headed",
+    "test:e2e:server": "npm run rebuild && node e2e-tests/test-server.js",
+    "test:e2e:visual": "npm run rebuild && cd e2e-tests && DEBUG_MODE=1 npx playwright test --headed",
    "lint": "turbo run lint",
    "lint:fix": "turbo run lint:fix",
    "compile": "turbo run compile",


WOW, debug mode is awesome. I think you should demo this!

mshanemc · 2025-09-08T14:52:50Z

packages/apex-ls/src/storage/StorageFactory.ts

+      typeof process !== 'undefined' &&
+      (process.env.NODE_ENV === 'test' ||
+        process.env.JEST_WORKER_ID !== undefined)
+    );


just below this line, what's the purpose of StorageFactor? I don't see it used outside of tests.

I was cleaning out overused factory patterns Cursor introduced and it probably just got missed on the clean up effort last week. https://gus.lightning.force.com/lightning/r/ADM_Work__c/a07EE00002Kp7ccYAB/view
I can continue to root out some of those artifacts on a different PR

mshanemc

no objections, your honor.

[well, I object to strategy pattern classes, but that's a different convo and a future PR 🤪 ]

kylewalke added 3 commits August 30, 2025 11:50

feat: e2e testing

d85a16c

feat: expanding to outline verification case

0faf642

fix: more tests for specifically outline view

fcf0e4d

kylewalke requested a review from a team as a code owner September 2, 2025 19:33

kylewalke requested a review from CristiCanizales September 2, 2025 19:33

kylewalke changed the base branch from main to tdx26/main September 2, 2025 19:33

kylewalke added 3 commits September 2, 2025 14:56

fix: removing some cruft

ac2118a

chore: lint fix and cleanup

c6fe687

fix: some structure

83a7f67

kylewalke requested a review from peternhale September 2, 2025 23:48

fix: minor

fbc8c2b

mshanemc reviewed Sep 3, 2025

View reviewed changes

kylewalke added 2 commits September 4, 2025 13:12

fix: pr feedback

5ca3dee

fix: ci fix

d7c3fd0

kylewalke force-pushed the kyledev/e2eTests branch from 40fda3b to 30bee76 Compare September 4, 2025 20:54

fix: improve e2e test CI environment compatibility

e4861f1

kylewalke force-pushed the kyledev/e2eTests branch from 30bee76 to e4861f1 Compare September 4, 2025 20:55

fix: CI fix

27273df

kylewalke force-pushed the kyledev/e2eTests branch from 630bd2a to 27273df Compare September 4, 2025 21:15

kylewalke added 2 commits September 4, 2025 14:20

kylewalke requested a review from mshanemc September 4, 2025 22:37

kylewalke added 4 commits September 5, 2025 11:34

fix: minor config change to fix up for demo

c10fe78

feat: testing CI changes

c5dfdf9

fix: tweak timers

29a6b11

fix: minor CI tweaking still

5b2e9af

mshanemc reviewed Sep 8, 2025

View reviewed changes

fix: pr feedback 2

2717088

mshanemc approved these changes Sep 9, 2025

View reviewed changes

fix: regex fix

e3512e6

kylewalke merged commit fdb86fa into tdx26/main Sep 9, 2025
3 checks passed

kylewalke deleted the kyledev/e2eTests branch September 9, 2025 22:03

@W-19444022@ E2E tests base level #121

@W-19444022@ E2E tests base level #121

Uh oh!

Conversation

kylewalke commented Sep 2, 2025

What does this PR do?

What issues does this PR fix or reference?

Uh oh!

mshanemc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylewalke Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylewalke Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylewalke Sep 9, 2025 •

edited

Loading

kylewalke Sep 3, 2025 •

edited

Loading